Fix "An input component must either have a 'aria-label' attribute..." error when a <label> is present by tjosepo · Pull Request #1230 · workleap/sg-orbit

Tommy Josépovic (tjosepo) · 2023-09-05T21:00:37Z

Summary

Removes the warning "An input component must either have a 'aria-label' attribute..." when a <label> is present.

What I did

I moved the validation logic inside a useEffect and add a check for isNilOrEmpty(input.labels).

I had to modify isNilOrEmpty to support arrays (and NodeList). Instead of checking value === "", it checks value.length === 0 (this works for strings, arrays and NodeLists).

How to test

Unit tests were added.

changeset-bot · 2023-09-05T21:00:40Z

🦋 Changeset detected

Latest commit: 871a9a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@orbit-ui/components	Patch
@sharegate/orbit-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify · 2023-09-05T21:03:56Z

✅ Deploy Preview for sg-orbit ready!

Name	Link
🔨 Latest commit	`871a9a3`
🔍 Latest deploy log	https://app.netlify.com/sites/sg-orbit/deploys/64f895583e67b00008156181
😎 Deploy Preview	https://deploy-preview-1230--sg-orbit.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2023-09-05T21:04:07Z

✅ Deploy Preview for sg-storybook ready!

Name	Link
🔨 Latest commit	`871a9a3`
🔍 Latest deploy log	https://app.netlify.com/sites/sg-storybook/deploys/64f89558d8cf0700088e1614
😎 Deploy Preview	https://deploy-preview-1230--sg-storybook.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Patrick Lafrance (patricklafrance) · 2023-09-06T00:02:12Z

packages/components/src/number-input/src/NumberInput.tsx

        value: inputValueRef.current
    });

+    useEffect(() => {


I believe this is textbook what React is now recommending NOT to do with useEffect: https://react.dev/reference/react/useEffect#caveats

Which part specifically are you concerned about?

Logging something in the console is the textbook definition of a side-effect in functional programming.

To check for the presence of a label, we need to access the ref to obtrain the HTMLInputElement. Using a useEffect or a callbackRef are the main ways to do it.

Patrick Lafrance (patricklafrance) · 2023-09-06T03:02:46Z

I am not sure I understand the use case correctly.

Does the following code would print a warning?

    <Field required>
        <Label>Username</Label>
        <TextInput placeholder="john@spacex.com" />
    </Field>

When there's a Field and a Label, the label id should be retrieved from the FieldContext by the following code of the Input component:

const [fieldProps] = useFieldInputProps();

fieldProps would then add to the Input props the aria-describedby and aria-labelledby properties, which should prevent the warning from being displayed.

From what I can see by the following test, the issue might be that the Input and the Label are not wrapped within a Field component.

Tommy Josépovic (tjosepo) · 2023-09-06T13:42:52Z

Does the following code would print a warning?

✅ I confirm that using <Field> worked without printing a warning.

I'll add it to the test scenarios.

I didn't know the <Field> component automatically added the accessibility properties.

To me, it still feel like a bug because using a label with htmlFor is accessible HTML, yet Orbit logs an error.

<>
    <Label htmlFor="test">Label</Label>
    <TextInput id="test" />
</>

I understand that labels are this is not as easily verifyable as using aria-label, aria-labelledby, and placeholder, since this forces us to move the validation logic after the component has been rendered on the page. This prevents us from checking accessibility during SSR. Altough, it is just as unit-testable.

I think it's a valid trade-off.

We can also argue against this change if we prefer people to use the <Field> component instead of manually passing in a label with htmlFor. In this case, we should update the warning message to mention using the <Field> component.

Patrick Lafrance (patricklafrance) · 2023-09-06T14:52:09Z

Thank for the reply Tommy Josépovic (@tjosepo). Yes, the intent is to always use the <Field> component to group together a Label and an Input. Great idea to update the warning message!

Tommy Josépovic (tjosepo) · 2023-09-06T14:53:35Z

Alright! I'll update my PR and go for it.

Tommy Josépovic (tjosepo) added 2 commits September 5, 2023 16:56

Remove accessibility warning when a label element is present

0feb677

Add changeset

a29cff6

Patrick Lafrance (patricklafrance) reviewed Sep 6, 2023

View reviewed changes

Add more tests

fa5066d

Tommy Josépovic (tjosepo) added 2 commits September 6, 2023 11:03

Revert label check

d5eaab8

Remove unused imports

871a9a3

Patrick Lafrance (patricklafrance) approved these changes Sep 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "An input component must either have a 'aria-label' attribute..." error when a <label> is present#1230

Fix "An input component must either have a 'aria-label' attribute..." error when a <label> is present#1230
Tommy Josépovic (tjosepo) wants to merge 5 commits intomasterfrom
bug-input-error-labels

Tommy Josépovic (tjosepo) commented Sep 5, 2023

Uh oh!

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

Uh oh!

netlify bot commented Sep 5, 2023 •

edited

Loading

Uh oh!

netlify bot commented Sep 5, 2023 •

edited

Loading

Uh oh!

Patrick Lafrance (patricklafrance) Sep 6, 2023 •

edited

Loading

Uh oh!

Tommy Josépovic (tjosepo) Sep 6, 2023

Uh oh!

Patrick Lafrance (patricklafrance) commented Sep 6, 2023 •

edited

Loading

Uh oh!

Tommy Josépovic (tjosepo) commented Sep 6, 2023 •

edited

Loading

Uh oh!

Patrick Lafrance (patricklafrance) commented Sep 6, 2023 •

edited

Loading

Uh oh!

Tommy Josépovic (tjosepo) commented Sep 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Tommy Josépovic (tjosepo) commented Sep 5, 2023

Summary

What I did

How to test

Uh oh!

changeset-bot bot commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

netlify bot commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for sg-orbit ready!

Uh oh!

netlify bot commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for sg-storybook ready!

Uh oh!

Patrick Lafrance (patricklafrance) Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tommy Josépovic (tjosepo) Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

Patrick Lafrance (patricklafrance) commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tommy Josépovic (tjosepo) commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patrick Lafrance (patricklafrance) commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tommy Josépovic (tjosepo) commented Sep 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot bot commented Sep 5, 2023 •

edited

Loading

netlify bot commented Sep 5, 2023 •

edited

Loading

netlify bot commented Sep 5, 2023 •

edited

Loading

Patrick Lafrance (patricklafrance) Sep 6, 2023 •

edited

Loading

Patrick Lafrance (patricklafrance) commented Sep 6, 2023 •

edited

Loading

Tommy Josépovic (tjosepo) commented Sep 6, 2023 •

edited

Loading

Patrick Lafrance (patricklafrance) commented Sep 6, 2023 •

edited

Loading